Skip to content

Fix can_view_submission ignoring ReviewerSettings configured in wagtail admin#4899

Merged
frjo merged 3 commits into
mainfrom
fix-reviewer-permission-issues
Jul 8, 2026
Merged

Fix can_view_submission ignoring ReviewerSettings configured in wagtail admin#4899
frjo merged 3 commits into
mainfrom
fix-reviewer-permission-issues

Conversation

@wes-otf

@wes-otf wes-otf commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Fixed a permission bug that ignored the ReviewerSettings that were set in Wagtail admin.

By default (no settings configured) a reviewer should have access to all submissions. But in Wagtail, setting can be configured to scope a reviewer's specific access to things like assigned applications, specific application outcomes, etc. This was not being considered by can_view_submission, which would give reviewers access to things like attachments on applications they wouldn't otherwise have access to.

The logic for ReviewerSettings was implemented on the ReviewerSubmissionDetailView alone so the detail view would account for these settings but not the can_view_submission function itself.

Thank you to https://github.com/Santoshkumarpuppala for discovering and responsibly disclosing this bug!

Test Steps

  • Ensure that with no ReviewerSettings configured/enabled in Wagtail admin, reviewers are able to access all submissions (both in detail view and see them displayed in the All Submissions view)
  • Ensure that with ReviewSettings configured, all permissions act as expected (ie. if a submission cannot be viewed, confirmed associated files also return a 403)

@wes-otf wes-otf requested a review from frjo July 7, 2026 15:56
@wes-otf wes-otf added Type: Bug Bugs! Things that are broken :-/ Type: Patch Mini change, used in release drafter labels Jul 7, 2026
Comment thread hypha/apply/funds/permissions.py Outdated
or submission.user == user
or user.is_reviewer
or submission.co_applicants.filter(user=user).exists()
):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be most correct to move these above if user.is_reviewer:.

If e.g. a user is staff and reviewer the staff role should take precedence.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally makes sense!

if reviewer_settings.use_settings:
qs = qs.for_reviewer_settings(request.user, reviewer_settings)
else:
qs = qs.filter(reviewers=request.user)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was good to get cleared up.

@wes-otf wes-otf requested a review from frjo July 8, 2026 17:03
@wes-otf

wes-otf commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

should be fixed now @frjo !

@frjo frjo merged commit 2dcbcfe into main Jul 8, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Bug Bugs! Things that are broken :-/ Type: Patch Mini change, used in release drafter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants